-
Notifications
You must be signed in to change notification settings - Fork 33
Top level API #76
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Top level API #76
Conversation
| => Handler m api -- ^ Handler for the query. This links the query to the code you've written to handle it. | ||
| -> QueryDocument VariableValue -- ^ A validated query document. Build one with 'compileQuery'. | ||
| -> Maybe Name -- ^ An optional name. If 'Nothing', then executes the only operation in the query. If @Just "something"@, executes the query named @"something". | ||
| -> VariableValues -- ^ Values for variables defined in the query document. A map of 'Variable' to 'Value'. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add how to construct VariableValues in the docstring?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started to do this and realised that we don't have a great story for it. Separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm looking for how to accomplish this now, but don't see any new information. Is the mentioned PR available?
tests/Examples/InputObject.hs
Outdated
| -- >>> import Data.Aeson (encode) | ||
| -- >>> import GraphQL.Value.ToValue (ToValue(..)) | ||
|
|
||
| -- TODO: jml thinks it's a bit confusing to have `show` output in these |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure what the TODO is here. Tests seem fine to me?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The braces & quotes within the string within the JSON confuse me a little syntactically. Hard to explain beyond what I've already commented. Have just done the TODO in 88056e4. Let me know if a) that clarifies what I meant by the TODO, b) you think that makes the example more effective.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will merge this change as-is, but please do let me know if you want it changed again.
|
|
||
| -- | Execute a GraphQL query. | ||
| executeQuery | ||
| :: forall api m. (HasGraph m api, Applicative m) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed adding an Object n i f ~ api constraint to make sure executeQuery can run only on Objects. Up to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Separate PR? Also, overnight I thought that that an IsQueryRoot constraint (with a resolveRoot :: Handler m a -> m Object method) wouldn't be such a bad thing.
As discussed, new functions in top-level
GraphQLmodule:executeQueryinterpretQueryinterpretAnonymousQueryThese all have copious (but not necessarily good) documentation. Please check the Haddock documentation generated by Circle to confirm that it's readable in context.
I've deleted the filesystem example, as discussed. I've updated the other examples to refer to
interpretAnonymousQuery, along with making some other changes to improve their documentation.getOperationandSelectionSetare no longer exposed at the top-level.TypeApiTests(which should really be renamed toResolverTests) also got a bit of a tidy up. I'm not sure what the point of the custom monad is any more, but it's there anyway.Fixes #60